🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18855

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures, 3 Unrelated Failures

As of commit 3f8d2ce with merge base e84a418 ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@metascroy has exported this pull request. If you are a Meta employee, you can view the originating Diff in D100710833.

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@metascroy has imported this pull request. If you are a Meta employee, you can view this in D100710833.

@claude review this code

Claude finished @metascroy's task in 2m 9s —— View job

Code Review: Add BackendOption loading to Swift bindings

• Read and analyze all changed files
• Review ObjC headers and implementations
• Review Swift extensions and tests
• Review Package.swift and build script changes
• Check for issues flagged in existing review comments
• Post comprehensive review feedback

Overall Assessment

This is a well-structured addition that properly bridges the C++ LoadBackendOptionsMap API to Swift/ObjC. The lifetime management (retaining BackendOptionsMap via ARC so the C++ borrowed pointer stays valid) is the key design decision and it's handled correctly. The test coverage is thorough, especially the regression tests for the dangling-pointer scenario.

Issues Flagged in Existing Review Comments

I agree with the self-review feedback from @metascroy:

1. requireFixture message is too specific (ModuleTest.swift:31)

The error message hardcodes generate_coreml_test_models.py but the helper is generic. Either make the message parametric, or just say "fixture not found":

let message = "\(name).\(type) not bundled."

Fix this →

2. testLoadWithBackendOptions uses add.pte — does this pass? (ModuleTest.swift:267)

The test at line 266 uses the add.pte fixture (which has zero delegates) with a "SomeBackend" option. This will pass because the runtime simply ignores backend options for backends that aren't present in the model — the options map is consulted per-delegate during load_method. So the test exercises the API path without triggering a real delegate lookup. This is fine for API smoke-testing but the comment or test name could clarify it's testing the "options are accepted without error" path, not that options are applied.

3. Long regression comment before testLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel (ModuleTest.swift:327-339)

The 10-line comment block is very detailed about internal crash mechanics. Consider trimming to 2-3 lines explaining what scenario this guards against, and leave the full debugging narrative in the commit message. Per CLAUDE.md: "Minimal comments; code should be self-documenting."

Fix this →

4. initWithOptions vs mapWithOptions (ExecuTorchBackendOptionsMap.h:54)

Having both is standard ObjC convention (designated initializer + class factory), but the factory method doesn't add value here since it's a one-liner that calls the init. If you want to keep the ObjC pattern, it's fine — but if this is primarily consumed via Swift (where BackendOptionsMap(options:) maps to the init), the factory could be removed to reduce API surface.

5. "this refactor" in comment (ExecuTorchModule.mm:263-264)

The comment says "...the dangling-pointer bug class this refactor was created to eliminate." The phrase "this refactor" references a temporal event and will become meaningless over time. Rewrite to state the invariant without referencing the PR:

// Resetting to nil would release the C++ map underneath _module's
// borrowed pointer, causing a dangling-pointer crash.

Fix this →

Additional Findings

6. Integer range check is a no-op on LP64 (ExecuTorchBackendOptionsMap.mm:75-77)

if (opt.intValue < INT_MIN || opt.intValue > INT_MAX) {
  return Error::InvalidArgument;
}

On 64-bit Apple platforms, NSInteger is int64_t and INT_MIN/INT_MAX are 32-bit constants, so this check works correctly. However, on a hypothetical 32-bit build, NSInteger == int32_t == INT_MIN..INT_MAX, making the condition always false (dead code). The test testBackendOptionsMapValidation passes Int(Int32.max) + 1 which requires 64-bit Int. This is fine for Apple's current 64-bit-only platforms, but worth noting.

7. BackendOptionsMap Swift API mismatch with PR description (ExecuTorch+Module.swift:62-67)

The PR description shows:

try module.load(backendOptions: [
  "CoreMLBackend": [BackendOption("compute_unit", "cpu_and_gpu")]
])

But the actual Swift API requires a two-step construction:

let options = try BackendOptionsMap(options: [...])
try module.load(options)

This is arguably better (construct once, reuse many times), but the PR description is misleading. Consider adding a convenience load(backendOptions:) overload that accepts the raw dictionary directly, or updating the PR description.

8. loadMethod:options: does NOT retain options (ExecuTorchModule.mm:375-395)

The comment says the C++ Module::load_method "uses it within the same call stack and does not store it." This should be verified — if the C++ Module's load_method stores the LoadBackendOptionsMap* for deferred use (as load() does), this would be a dangling-pointer bug. Looking at the call: _module->load_method(...) with the map pointer — if the method is loaded synchronously and doesn't store the map, this is safe. But it's a subtle invariant that could break if the C++ API changes.

9. Missing NS_ASSUME_NONNULL_END visual balance (ExecuTorchBackendOption.mm)

Minor: The .mm file doesn't use NS_ASSUME_NONNULL_BEGIN/END unlike the header. This is fine (it's an implementation file), just noting for consistency.

10. Test testBackendOptionsMapReusedAcrossModules depends on CoreML fixture (ModuleTest.swift:433)

This test uses requireFixture("add_coreml", ...) which will be skipped on local dev machines without CoreML python deps. The non-CoreML variant of this test (reuse across modules with add.pte) isn't present. Consider adding one with the plain add.pte fixture if the goal is to test ARC-based reuse regardless of backend.

Code Quality

• The ObjC class design is clean: immutable value types, proper isEqual:/hash, defensive description overrides.
• The C++ bridge in buildBackendOptionsMap() correctly handles lifetime of the std::vector<std::vector<BackendOption>> storage backing the Spans.
• The Package.swift conditional resource inclusion is pragmatic for the CI-only fixture.
• The .gitignore files and symlink strategy for ObjC test resources are well thought out.

Summary

The implementation is solid. The main actionable items from the self-review are straightforward comment/message fixes. The most architecturally important thing to verify is point #8 — ensuring load_method's C++ contract truly doesn't store the map pointer.

@claude review this code

Claude finished @metascroy's task in 2m 34s —— View job

Code Review: Add BackendOption loading to Swift bindings

• Read and analyze all changed files
• Review ObjC headers and implementations
• Review Swift extensions and tests
• Review Package.swift and build script changes
• Check for issues flagged in existing review comments
• Post comprehensive review feedback

Overall Assessment

This is a well-structured addition that correctly bridges the C++ LoadBackendOptionsMap API to Swift/ObjC. The key design decision — retaining BackendOptionsMap via ARC so the C++ Module::backend_options_ raw pointer (stored at module.cpp:195) stays valid through lazy load_method calls (consumed at module.cpp:363) — is sound and well-documented.

The code quality is high: immutable value types, proper isEqual:/hash, defensive description overrides, and thorough test coverage including the dangling-pointer regression path.

Issues from Self-Review Comments

1. requireFixture error message — Already fixed. The current code at ModuleTest.swift:30 uses the generic "\(name).\(type) not bundled." message. No action needed.

2. mapWithOptions: factory method (ExecuTorchBackendOptionsMap.h:61-63)

The class factory +mapWithOptions:error: is a standard ObjC pattern but since this class is primarily consumed via Swift (where BackendOptionsMap(options:) maps to the designated init), it adds API surface without adding value. Fine to keep for ObjC convention, but could be removed if you want a leaner API.

3. _loadedBackendOptions ivar comment (ExecuTorchModule.mm:255-269)

The comment block is well-written and documents the key invariant clearly. However, the original review flagged "this refactor" language — I see it's already been removed in the current version. The current text is clean.

Additional Findings

4. Potential issue: loadMethod:options: retains options but load_method stores nothing

At ExecuTorchModule.mm:384, _loadedBackendOptions = options overwrites the previously retained options. This is fine because:

• C++ Module::load_method() at module.cpp:357 uses the passed backend_options pointer only within the call scope to initialize the method.
• Once the method is loaded, the pointer is no longer needed.

But there's a subtle edge case: if a user calls loadWithOptions: (which stores backend_options_ for future lazy load_method calls) and then later calls loadMethod:options: with different options, the _loadedBackendOptions assignment at line 384 would release the first options object — but _module->backend_options_ still points at the first object's C++ storage. This would be a use-after-free if another method is lazily loaded later via forward().

Scenario:

[module loadWithOptions:optionsA error:&err];  // _module->backend_options_ = &optionsA.cppMap
// No methods loaded yet (lazy)
[module loadMethod:@"preprocess" options:optionsB error:&err];
// _loadedBackendOptions = optionsB → releases optionsA
// _module->backend_options_ still points at freed optionsA storage!
[module forward:...];  // lazy loads "forward" via backend_options_ → dangling pointer

Recommendation: Either:

• Store both (e.g., accumulate into an array), or
• Assert/document that loadMethod:options: must not be called after loadWithOptions: on the same module, or
• Have loadMethod:options: NOT overwrite _loadedBackendOptions (since the C++ load_method path consumes it synchronously and doesn't store it).

The simplest fix is option 3 — don't assign _loadedBackendOptions = options in loadMethod:options: since load_method doesn't store the pointer:

- (BOOL)loadMethod:(NSString *)methodName
           options:(ExecuTorchBackendOptionsMap *)options
             error:(NSError **)error {
  NSParameterAssert(options);
  // load_method consumes backend_options synchronously; no retention needed.
  const auto errorCode = _module->load_method(methodName.UTF8String,
                                               /*planned_memory=*/nullptr,
                                               /*event_tracer=*/nullptr,
                                               [options cppMap]);
  ...
}

But then the caller must ensure options stays alive for the duration of the call (which ARC guarantees since it's a parameter). So this is actually safe and simpler.

Fix this →

5. Integer range check is a no-op on 32-bit (ExecuTorchBackendOptionsMap.mm:75-77)

if (opt.intValue < INT_MIN || opt.intValue > INT_MAX) {
  return Error::InvalidArgument;
}

On 64-bit Apple platforms (the only targets currently supported), NSInteger is int64_t so this works correctly. On a hypothetical 32-bit build, NSInteger == int32_t and the check becomes tautologically false. Not a real issue today, but worth a brief note if 32-bit ever matters.

6. std::move(map) may not do what's expected (ExecuTorchBackendOptionsMap.mm:141)

_map = std::move(map);

If LoadBackendOptionsMap internally stores Span<BackendOption> objects that point into storage's vectors, and _storage = std::move(storage) runs on line 140 first, the spans inside map/_map still point at the (now moved) _storage vectors' heap buffers. This is correct because std::vector move preserves the data pointer. But if LoadBackendOptionsMap's move constructor does anything beyond a shallow member copy (e.g., recalculates span pointers), this could break. The existing comment at lines 36-40 already explains this — just flagging that this is a fragile invariant worth a unit test (which testLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel effectively provides).

7. Missing test for loadMethod:options: Swift path

The Swift extension exposes module.load("forward", options: opts) but there's no test exercising this path. Consider adding a brief test that calls module.load("forward", options: opts) followed by module.forward(...) to verify the loadMethod:options: ObjC bridge works end-to-end.

8. testBackendOptionsMapValidation depends on 64-bit Int (ModuleTest.swift:342)

BackendOption("too_big", Int(Int32.max) + 1)

This would overflow on a 32-bit platform. Fine for Apple's current 64-bit-only targets, just noting for completeness.

Minor Nits

• ExecuTorchBackendOption.mm:80: The (null) case in the string description is unreachable given the init enforces non-nil stringValue, but the defensive coding is fine.
• generate_coreml_test_models.py: Clean and minimal. The os.path.dirname(__file__) approach is good for finding the output directory.
• The .gitignore files and symlink strategy for ObjC test resources are well thought out and avoid committing generated artifacts.

Summary

The implementation is solid. The one actionable concern is point #4 — the loadMethod:options: assignment to _loadedBackendOptions could cause a dangling pointer if called after loadWithOptions: with different options. The fix is straightforward (remove the assignment since load_method doesn't store the pointer). Everything else is clean and well-considered.

@claude review this PR

Claude finished @metascroy's task in 2m 32s —— View job

Code Review: Add BackendOption loading to Swift bindings

• Read and analyze all changed files
• Review ObjC headers and implementations
• Review Swift extensions and tests
• Review Package.swift and build script changes
• Post comprehensive review feedback

Overall Assessment

This is a well-designed addition. The key architectural decision — retaining BackendOptionsMap via ARC so the C++ Module::backend_options_ raw pointer (stored at module.cpp:195) remains valid through lazy load_method calls (consumed at module.cpp:363) — is correct and thoroughly tested, including the testMixedLoadWithOptionsAndLoadMethodWithOptionsOnMultiMethodModel regression test with weak reference tracking.

The code quality is high: immutable value types, proper isEqual:/hash, validation at construction time, and the ObjC/Swift layering is clean.

Critical Issue

loadMethod:options: must not overwrite _loadedBackendOptions — Already handled correctly.

I verified the implementation at ExecuTorchModule.mm:374-396. The comment at line 378-385 explicitly documents why _loadedBackendOptions is NOT assigned here, avoiding the use-after-free scenario where loadWithOptions: installs optionsA into _module->backend_options_, and a subsequent loadMethod:options: with optionsB would release optionsA's storage. This is the correct design — the testMixedLoadWithOptionsAndLoadMethodWithOptionsOnMultiMethodModel test validates it deterministically via the weakA assertion.

Suggestions

1. mapWithOptions: factory method adds little value (ExecuTorchBackendOptionsMap.h:61-63)

Since Swift consumers use BackendOptionsMap(options:) (which maps to the designated init), the class factory is only useful for ObjC callers who prefer +mapWith style. It's used in the ObjC tests, so it earns its keep there. No change needed, just noting it's a stylistic choice.

2. Move semantics of _storage and _map deserve a targeted unit test (ExecuTorchBackendOptionsMap.mm:144-152)

The comment at lines 145-151 correctly explains why _storage = std::move(storage) must come before _map = std::move(map) — the Spans inside map point into the inner vectors' heap buffers, and std::vector move preserves data() pointers. This is fragile: if LoadBackendOptionsMap's move constructor ever recomputes span pointers, this would break silently. The end-to-end CoreML test exercises this path (as the comment notes), but a targeted test that validates the pointer stability more directly would add confidence.

3. buildBackendOptionsMap could return a more specific error for different failure modes (ExecuTorchBackendOptionsMap.mm:41-114)

Currently all validation failures return the same Error::InvalidArgument. The caller maps this to a single NSError. Consider differentiating (e.g., via userInfo) to distinguish "integer overflow" vs "key too long" vs "value too long" so callers can provide actionable error messages. Low priority — the current behavior is acceptable for v1.

4. The options property returns a snapshot, not the live backing (ExecuTorchBackendOptionsMap.mm:171-173)

This is the correct choice for an immutable API, but worth noting that the snapshot is built eagerly at init time (lines 158-162). For large option dictionaries this doubles memory briefly during construction. Not a real concern for the typical 1-5 options per backend, but good to be aware of.

5. Test coverage gap: no non-CoreML test for load(options) (ModuleTest.swift:266-286)

All load(options) tests that actually exercise the lazy load path depend on CoreML fixtures (add_coreml.pte, add_mul_coreml.pte). The non-delegated add.pte model would silently ignore backend options (since it has no delegates), so it can't test the option lookup. This is acceptable — the runtime simply ignores options for absent backends — but it means local developers without CoreML deps will get all backend-option tests skipped. Consider adding a brief test with add.pte that verifies load(options) doesn't error even when no matching backend exists, to exercise the API path without CoreML:

func testLoadWithBackendOptionsOnNonDelegatedModel() throws {
    guard let modelPath = resourceBundle.path(forResource: "add", ofType: "pte") else {
        XCTFail("Couldn't find the model file")
        return
    }
    let module = Module(filePath: modelPath)
    let options = try BackendOptionsMap(options: [
        "SomeBackend": [BackendOption("key", "value")]
    ])
    XCTAssertNoThrow(try module.load(options))
    let outputs: [Value] = try module.forward(Tensor<Float>([1]), Tensor<Float>([2]))
    XCTAssertEqual(outputs.first?.tensor(), Tensor([Float(3)]))
}

Add this test →

6. Package.swift uses Foundation import for FileManager (Package.swift:21)

This is valid but somewhat unusual for a Package.swift manifest. SwiftPM processes the manifest in a sandbox, and FileManager.default.fileExists(atPath:) relies on the path being relative to the package root (which it is, since SwiftPM evaluates manifests from the repo root). This works correctly.

7. ObjC test requireFixture and Swift test requireFixture have slightly different semantics

The Swift version (ModuleTest.swift:26-42) throws XCTSkip locally or a plain NSError in CI. The ObjC version (ModuleTestObjC.m:32-46) calls XCTFail in CI (which does not throw — the test continues!) and then returns nil. The ObjC caller pattern if (!modelPath) return; handles this correctly, but it's a subtle difference from the Swift version where throw halts execution. Both are correct, just different idioms for their respective languages.

Minor Nits

• ExecuTorchBackendOption.mm:80: The (null) fallback in the string description case is unreachable since the init enforces non-nil stringValue. Defensive coding is fine here.
• ExecuTorchBackendOption.mm:66-67: strncpy with the exact kMaxOptionKeyLength - 1 bound is correct (null terminator set on line 67). The strlen check on line 63 guarantees no truncation.
• generate_coreml_test_models.py: Clean and minimal. The multi-method model generation using to_edge_transform_and_lower({"forward": ep_add, "mul": ep_mul}, ...) is a neat pattern.

Summary

The implementation is solid and production-ready. The lifetime management is the hardest part of this design and it's handled correctly with proper documentation and regression tests. The main suggestion is adding a non-CoreML smoke test so local developers get at least one backend-options API test running without CoreML python deps.